-
Notifications
You must be signed in to change notification settings - Fork 2.5k
fix: add GitHub Codespaces support for browser tool #7992
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote this code twice because copy-paste is my primary design pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These helper methods (isCodespacesEnvironment() and checkSystemChromium()) are duplicated in both BrowserSession.ts and UrlContentFetcher.ts. Could we extract them into a shared utility file like src/utils/browserEnvironment.ts to follow DRY principles?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this duplication intentional? The --disable-gpu flag appears on line 113 and again on line 119. Same with --disable-dev-shm-usage on lines 110 and 119. Could we deduplicate these args?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message includes a hardcoded list of packages that might become outdated. Could we consider either:
- Referencing the devcontainer.json file
- Linking to documentation
- Creating a constant that's shared with the devcontainer configuration?
.devcontainer/devcontainer.json
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add a comment explaining why these environment variables are needed? It would help future maintainers understand that:
- PUPPETEER_SKIP_CHROMIUM_DOWNLOAD: Prevents downloading Chromium since we're using the system-installed version
- PUPPETEER_EXECUTABLE_PATH: Points to the system Chromium installation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests verify the browser launch arguments but don't cover the new Codespaces-specific logic. Should we add tests for:
- isCodespacesEnvironment() detection
- checkSystemChromium() fallback behavior
- The enhanced error messages in Codespaces?
- Add automatic detection of Codespaces environment - Implement auto-fix using 'sudo apt --fix-broken install' when browser launch fails - Add retry logic after fixing dependencies - Create devcontainer configuration for seamless Codespaces setup - Add comprehensive tests for new functionality Fixes #7990
98ddcbb to
9abfa0c
Compare
|
Updated implementation based on @LousyBook94's suggestion to use The new approach:
This should provide a more robust solution that automatically fixes the issue without requiring manual intervention. |
Description
This PR attempts to address Issue #7990 by adding proper support for the browser tool in GitHub Codespaces environments.
Problem
The browser tool was failing in GitHub Codespaces with the error:
This occurs because the Chromium binary downloaded by puppeteer-chromium-resolver lacks required system dependencies in containerized environments.
Solution
Devcontainer Configuration: Added
.devcontainer/devcontainer.jsonwith all necessary Chromium dependencies that will be automatically installed when a Codespace is created or rebuilt.Runtime Detection: Added environment detection to identify when running in Codespaces and attempt to use system-installed Chromium first before falling back to downloading.
Enhanced Error Messages: When the browser fails to launch in Codespaces, users now receive helpful instructions on how to fix the issue.
Proper Sandbox Flags: Added appropriate browser launch flags for containerized/Linux environments to ensure compatibility.
Changes
.devcontainer/devcontainer.jsonwith Chromium dependenciessrc/services/browser/BrowserSession.tsto detect Codespaces and use system Chromiumsrc/services/browser/UrlContentFetcher.tswith the same enhancementsTesting
Notes
The review identified an opportunity to extract the duplicated Codespaces detection logic into a shared utility, but this can be addressed in a follow-up PR if desired.
Fixes #7990
Important
Adds GitHub Codespaces support by handling Chromium dependencies and errors in
BrowserSession.tsandUrlContentFetcher.ts..devcontainer/devcontainer.jsonto install Chromium dependencies in Codespaces.BrowserSession.tsandUrlContentFetcher.tsto use system Chromium if available.BrowserSession.tsandUrlContentFetcher.ts.isCodespacesEnvironment(),fixCodespaceDependencies(), andisMissingDependencyError()incodespaceUtils.ts.UrlContentFetcher.spec.tsto expect new browser arguments.codespaceUtils.spec.tsto test new utility functions.This description was created by
for 9abfa0c. You can customize this summary. It will automatically update as commits are pushed.